feat: Add client validation for structuredContent #302
Conversation
mcp-test/src/main/java/io/modelcontextprotocol/server/.LCKAbstractMcpSyncServerTests.java~
Outdated
Show resolved
Hide resolved
mcp/pom.xml
Outdated
| <!-- Json validator dependency --> | ||
| <dependency> | ||
| <groupId>com.networknt</groupId> | ||
| <artifactId>json-schema-validator</artifactId> | ||
| <version>1.5.7</version> | ||
| </dependency> |
There was a problem hiding this comment.
Worth getting extra clarification from maintainers on if we want schema validation as part of this or a separate effort, along the lines of the discussion in #271.
There was a problem hiding this comment.
Discussed some of the issues offline with @LucaButBoring. I wanted to get clarity around the scope of the work on the issue that I had created. There are a few questions that we might want to answer regarding the output validation:
- Do we want validation to be part of this PR or do we want to separate it out and think more about how we can cache/validate the results?
- Should cache be in the client class or should we construct a separate cache class?
- Should there be refresh intervals for cache invalidation? How else can we deal with it and should it be configurable by the client?
- Is a Map the best way to represent cache or should we opt for an in-memory cache implementation like Guava Cache?
Would like inputs from the maintainers to decide on the best way to proceed.
mcp/src/main/java/io/modelcontextprotocol/client/McpAsyncClient.java
Outdated
Show resolved
Hide resolved
mcp/src/main/java/io/modelcontextprotocol/client/McpSyncClient.java
Outdated
Show resolved
Hide resolved
| catch (JsonProcessingException e) { | ||
| // Log error if output schema can't be parsed to prevent erroring out for | ||
| // successful call tool request | ||
| logger.error("Encountered exception when parsing outputSchema: {}", e); | ||
| } |
There was a problem hiding this comment.
Worth getting other opinions on this, I think this should be rethrown (potentially wrapped in McpError to keep it unchecked).
There was a problem hiding this comment.
I agree. For now, changing this to log a warning. This leaves the option open for the client side implementation to proceed as it deems fit. I would like feedback from others about what would be the ideal behavior in this scenario.
mcp/src/main/java/io/modelcontextprotocol/client/McpSyncClient.java
Outdated
Show resolved
Hide resolved
|
Just gave this a preliminary review by request, review done for now. |
|
Addressed all comments from @LucaButBoring barring a few that where I would like a discussion with maintainers/others from the community. |
| @@ -215,7 +244,54 @@ public Object ping() { | |||
| * Boolean indicating if the execution failed (true) or succeeded (false/absent) | |||
| */ | |||
| public McpSchema.CallToolResult callTool(McpSchema.CallToolRequest callToolRequest) { | |||
| return this.delegate.callTool(callToolRequest).block(); | |||
| McpSchema.CallToolResult result = this.delegate.callTool(callToolRequest).block(); | |||
There was a problem hiding this comment.
Note: Shift to sync instead.
pantanurag555
force-pushed
the
output-schema
branch
from
f088e75 to
1911386
Compare
pantanurag555
changed the title
|
Updated the PR to leverage the existing json validator and to perform only client-side validations. Added a new test file to mock transport for the sync client. |
| /** | ||
| * Cached tool output schemas. | ||
| */ | ||
| private final ConcurrentHashMap<String, Optional<Map<String, Object>>> toolsOutputSchemaCache; |
There was a problem hiding this comment.
Did you wind up looking at cache abstractions like we had discussed offline a while back? Not sure if that fell through or if we just don't want to do that anymore.
I just took a look at Guava's cache implementation and they actually recommend using Caffeine now.
Separate comment: we should have a comment explaining the Optional if we're keeping that. It's not obvious why you would want an Optional value without reading the full implementation.
There was a problem hiding this comment.
Didn't use any cache abstractions to keep the review simple for now and avoid additional dependencies. The cache implementation as part of JsonValidator was also a similar ConcurrentHashMap so wanted to stay close to that at least for the first review of the client side validation. Additionally, python SDK implementation also used a dict initially for the cache: modelcontextprotocol/python-sdk#685
I will make a note to add additional comments about Optional to better explain it in the context of the code.
|
Is there a reason this was rewritten to only apply to the sync client? This should probably be in the async client, which the sync client would then benefit from automatically. |
This was applied to the sync client based on offline feedback from @tzolov. The previous review had most of the implementation as part of async, however in order to refresh the tool cache, the implementation needs to block on listTools() which leads to mixing of sync and async. The feedback was to consolidate the entire implementation in sync to avoid that. |
Motivation and Context
This PR adds client-side validation for CallToolResult.structuredContent. The specification was introduced in modelcontextprotocol/modelcontextprotocol@03dc24b.
Resolves #285
Changes in this PR:
How Has This Been Tested?
Breaking Changes
None
Types of changes
Checklist